-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to SOFA 2015-02-09 #26
Conversation
@eteq - I guess it would make sense to start using the Galactic coordinates defined here since they are IAU sanctioned (once we merge into Astropy). |
@timj - thanks for the PR! |
The new galactic stuff is really good - it means we finally have an IAU-sanctioned path from ICRS to Galactic that doesn't depend on FK4. The start of a new era of precision Galactic coordinates? :) |
@astrofrog - I'm not 100% sure we want to switch to using this... the problem isn't so much the lack of an IAU standard now as much as the lack of a standard in the past. That is, if ERFA's approach to e.g. the e-terms than what has been assumed in most cases up until now, we may want to leave in our implementation. Will have to look more closely. Regardless, thanks @timj for doing this - will review now. |
@eteq - I think that for ICRS <-> Galactic there were several different conventions before anyway so changing to the IAU sanctioned one would be good (no one trusts old galactic coordinates to better than a fraction of an arcsec anyway). In fact, we should probably make sure everyone switches to that specific convention for the future. For e-terms and things like that, we've actually been doing pretty well compared to other codes, so I'd be surprised if the IAU functions give different results from most other codes. We should check all this though! |
@@ -228,7 +229,7 @@ int eraDat(int iy, int im, int id, double fd, double *deltat ) | |||
if (i < 0) return -5; | |||
|
|||
/* Get the Delta(AT). */ | |||
da = changes[(i < 0) ? 0 : i].delat; | |||
da = changes[i].delat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change undoes #14, which is a fix we want. My impression was that this was supposed to be included in SOFA, but apparently not... but either way, we do not want to make this particular change. So can you just put back the
da = changes[(i < 0) ? 0 : i].delat;
that was there before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at that but you had already put in the line 228 change that cause the function to return early if i
is negative so why are we checking the variable again immediately afterwards. This way we minimize the changes between ERFA and SOFA. It would be easier if there was a branch for "pure" SOFA->ERFA and then a branch with the local changes. Then merges would be done by git rather than manually trying to keep them straight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timj - @mdboom should have a little bit more insight as the author of #14 . I think the main point was to avoid compiler warnings that were causing problems somewhere. (And this is the sort of thing we are willing to keep separate between ERFA and SOFA.)
Even if we decide to drop this change, though, the README would need to be updated to reflect this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timj: We only need one of the fixes here. The return if i is negative is the better one (but should still probably be upstream in SOFA if it isn't -- don't have time to check right now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. That's why I only kept the first check for a negative number.
The change was added in 506f302 which is part of the previous SOFA release. It looks to me like they edited the current dat.c
from an old version of the source and not the "2013-12-02_d" release. The version of dat.c
here does include the previous patch and the new version and correctly drops the local ERFA hack. I'll send a note to see if the dropping of the -5
error condition was deliberate. I will also update the README to say that the local patch is no longer needed (at least in the stated form).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds good. If there ends up being no local change necessary at all, then you can just remove the section about that change from the README entirely.
@astrofrog - definitely, and it's not really relevant until this is merged, anyway. |
@timj - actually, I just realized something relevant to the discussion of what to do with (That said, it probably makes sense to do a release including this shortly after merging and I can always update it then, so it doesn't matter that much.) |
Ah. You are right. It's not really my job to bump the version numbers. That's the job of the release manager. I'll remove the |
The compiler warning in dat.c was fixed in commit 506f302
I've pushed the change to the README and reverted the |
Ok, sounds good @timj. Lets give, say a week or so to hear what SOFA thinks on this? (Perhaps pinging @scottransom will help grease the wheels a bit?) |
@timj: Who did you contact at SOFA? I haven't heard about this at all. Patrick Wallace is the person who is doing most of the actual code development. If you don't mind, can you please forward to me what you sent? I can definitely try to nudge as appropriate. |
I used the contact email on the SOFA web page. I'm more than happy to email Pat directly -- we know each other well. |
The initial SOFA release had inadvertently removed a prior compiler warning fix.
SOFA inform me that it was a mistake and a new release will be made shortly. Pat has sent me the corrected version which I've now included in this PR. |
Ok, great. I think this is good except for one thing: should we wait until the official SOFA release with that fix so that we can make it clear this is based on that version? @timj, did you get a sense of whether "shortly" means a few days vs. a few months? |
The modified code has been submitted by Pat. The reply I got said "not too distant future". I don't see any harm in releasing this version as is -- it does have the fix from the previous SOFA release in it that is going to be in the next release so this version is technically less broken than the official version but in a way that has been approved by SOFA. I can add a little note to the README if you want to be explicit? |
@timj - ok, yeah, that's true - just replace the bullet point that currently says "None" to briefly note this, and that it's planned for a future SOFA. Then we can merge this and I can run through a release. |
I've pushed the README tweak. |
Ok, great, thanks @timj. Will merge this now, and go through the release sometime next week. Feel free to ping me if it hasn't happened... |
Includes minor change to README (last commit in the merge)
@timj sorry to arrive late to this. It seems sofa 20150209 has removed some functions: If somebody is in contact with the SOFA people, please tell them that backwards compatibility is important. |
Are you sure they've gone? Their definitions moved around in the |
@timj Yes, they are there... How is that in the PR is a |
@@ -277,10 +279,6 @@ double eraGst06a(double uta, double utb, double tta, double ttb); | |||
double eraGst94(double uta, double utb); | |||
|
|||
/* Astronomy/SpaceMotion */ | |||
int eraPmsafe(double ra1, double dec1, double pmr1, double pmd1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, was this function signature duplicated?
Because the function was defined twice... |
@timj oh, fine! |
Syncs up ERFA with release 2015-02-09 of SOFA.
I have attempted to retain the local patch to
dat.c
.The change looks larger than it really is because of the change in copyright date.
SOFA release notes: